Skip to content

Rustify ci/build_and_test.* scripts#6855

Merged
ytmimi merged 7 commits intorust-lang:mainfrom
GuillaumeGomez:migrate-to-rust
Apr 28, 2026
Merged

Rustify ci/build_and_test.* scripts#6855
ytmimi merged 7 commits intorust-lang:mainfrom
GuillaumeGomez:migrate-to-rust

Conversation

@GuillaumeGomez
Copy link
Copy Markdown
Member

Follow-up of #6817.

I merged both scripts so they are both part of ci/ci-integration rust binary. To pick which one to run, you need to pass either integration or build-and-test command line argument. In the same way, I replaced the INTEGRATION environment variable with a command line argument.

cc @jieyouxu

@rustbot rustbot added A-CI Area: CI S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Apr 3, 2026
@GuillaumeGomez
Copy link
Copy Markdown
Member Author

GuillaumeGomez commented Apr 3, 2026

Ignorant question but why is the config_proc_macro test trying to remove the ci-integration.exe binary file on windows? I didn't find anywhere in the repository a call to remove files in the out dir...

@jieyouxu jieyouxu self-assigned this Apr 3, 2026
@matthewhughes934
Copy link
Copy Markdown
Contributor

matthewhughes934 commented Apr 3, 2026

Ignorant question but why is the config_proc_macro test trying to remove the ci-integration.exe binary file on windows? I didn't find anywhere in the repository a call to remove files in the out dir...

Looking at the logs, that errors during the cargo build step, following:

  • The action runs cargo run --bin ci-integration build-and-test so target/debug/ci-integration.exe is built
  • That binary then runs cargo build --locked . with a different env (RUSTFLAGS='-D warnings') so it triggers a rebuild of target/debug/ci-integration.exe

So I think that's why it tries to remove it (because it's rebuilding) but not why that would cause an error. Unless Windows doesn't like it when a process deletes the file that spawned it

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

Interesting. But then why is it only an issue on Windows CI?

@matthewhughes934
Copy link
Copy Markdown
Contributor

matthewhughes934 commented Apr 3, 2026

Interesting. But then why is it only an issue on Windows CI?

I'm starting to suspect my hunch

... Windows doesn't like it when a process deletes the file that spawned it

is correct, but I don't know enough about Windows internals (nor handy access to a Windows machine) to be certain.

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

Writing down the process:

  1. We run cargo run --bin ci-integration build-and-test
  2. It sets the environment flags, including RUSTFLAGS="-D warnings"
  3. We run cargo build --locked
  4. We run cargo test
  5. We go into the config_proc_macro folder and run cargo build --locked. At this step it fails. But it's being run with the same environment variables. Hence my confusion.

@matthewhughes934
Copy link
Copy Markdown
Contributor

matthewhughes934 commented Apr 3, 2026

Writing down the process:

  1. We run cargo run --bin ci-integration build-and-test

  2. It sets the environment flags, including RUSTFLAGS="-D warnings"

  3. We run cargo build --locked

I think this is where we error out (some extra logging/reporting of the args being run in the error might help locate it), I see in the logs:

  1. A bunch of download+compile logs ending with Running target\debug\ci-integration.exe build-and-test (i.e. building that before running it)
  2. The output of rustc -vV and cargo -v, that ends with See 'cargo help <command>' for more information on a specific command. (because I think the original script has a bug, and that should be cargo -V i.e. cargo --version, so it prints the cargo help output)
  3. A bunch of download+compile logs from the cargo build step, and then the error

Since there's no Finished dev profile .... output I don't think it ever finishes the first cargo build

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

Oh I think you're right. Hum... Gonna need to think about how to solve that...

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

Is the CI status report broken?

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

Ah no, github actions files were broken. Yeay status reporting. ^^'

@GuillaumeGomez GuillaumeGomez force-pushed the migrate-to-rust branch 2 times, most recently from 568044b to 55183ea Compare April 7, 2026 10:18
@jieyouxu
Copy link
Copy Markdown
Member

jieyouxu commented Apr 8, 2026

(Will review this weekend, didn't get to it last weekend)

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

No hurry, I still need to find a way to prevent cargo to rebuild ci-integration. Currently trying with exclude but that creates other issues. It's super annoying that we can't tell cargo "ignore this project if you rebuild".

@GuillaumeGomez GuillaumeGomez force-pushed the migrate-to-rust branch 2 times, most recently from 2ec9f0e to 3b3c9bc Compare April 10, 2026 13:37
@GuillaumeGomez
Copy link
Copy Markdown
Member Author

Finally made it work! \o/

Ready for review now @jieyouxu when you have time. ;)

Copy link
Copy Markdown
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, really sorry for the late review. Only some tiny nits
@rustbot author

View changes since this review

Comment thread ci/src/build_and_test.rs
Comment thread ci/src/build_and_test.rs
Comment thread .gitignore Outdated
@rustbot rustbot removed the S-waiting-on-review Status: awaiting review from the assignee but also interested parties. label Apr 28, 2026
@rustbot rustbot added the S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. label Apr 28, 2026
Copy link
Copy Markdown
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

View changes since this review

@jieyouxu jieyouxu added pr-ready-to-merge Status: PR is largely ready for merge, waiting for secondary review / last nits S-waiting-on-review Status: awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. labels Apr 28, 2026
@ytmimi ytmimi merged commit a413ea9 into rust-lang:main Apr 28, 2026
26 checks passed
@rustbot rustbot added release-notes Needs an associated changelog entry and removed S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Apr 28, 2026
@ytmimi ytmimi removed the pr-ready-to-merge Status: PR is largely ready for merge, waiting for secondary review / last nits label Apr 28, 2026
@GuillaumeGomez GuillaumeGomez deleted the migrate-to-rust branch April 28, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-CI Area: CI release-notes Needs an associated changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants